Skip to content

fix(mcp): reject conflicting exclusions on shared operations - #115566

Closed
Silthus wants to merge 1 commit into
PostHog:masterfrom
Silthus:upstream/mcp-shared-operation-exclusions
Closed

Silthus wants to merge 1 commit into
PostHog:masterfrom
Silthus:upstream/mcp-shared-operation-exclusions

Conversation

@Silthus

@Silthus Silthus commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Problem

  • An MCP tool author can silently ship the wrong request schema when two enabled tools share one API operation and list different exclude_params.
  • Codegen builds one schema per operation, so one tool's exclusions quietly apply to the other.

Supersedes #111366 (closed), rebased on current master. Fork handoff: Silthus/posthog#344.

Note

🔵 Merge risk: low · two-way door (merge-risk rubric)

  • Codegen tooling only. Every tool definition on master passes the new check, so no generated output changes.

Changes

  • MCP codegen now fails when enabled tools on a shared operation list different exclusions within one YAML file. The error names the operation and asks for matching exclude_params.
  • A missing list counts as empty. Order and duplicates do not matter, and disabled tools do not count.
  • Rejected alternative: per-tool exclusions on a shared operation. No tool needs them yet.
  • Mechanical: a note in implementing-mcp-tools.md.

How did you test this code?

Test rationale: the existing generate-tools tests generate one tool at a time, so they cannot see two tools overwrite each other. The new tool-definitions.test.ts cases fail on the old parser.

  • Ran services/mcp/tests/unit/tool-definitions.test.ts locally on the rebased branch: it passes.
  • Not run locally: full hogli build:openapi. The reviewer ran the check over all 69 tool YAML files on master, and none conflict.

Release status

  • No feature flag controls this change

Docs update

Updated docs/published/handbook/engineering/ai/implementing-mcp-tools.md with the new rule.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: T3 Code with Claude Code, Claude Opus 5.5 (claude-opus-5-5). Cross-family review: Codex, GPT-6.1 Sol (gpt-6.1-sol), high reasoning effort, read-only.

  • Rebuilt from the closed PR's diff on master 60187c17de7. The patch applied without changes.
  • Skills: writing-pr-descriptions, writing-tests, merge-risk.
  • Review: no findings.
  • Duplicate search found no open PR for this check. fix(mcp): fail codegen when a yaml setting names no field #112861 extends the no-field check to other settings, which is a different rule.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@trunk-io

trunk-io Bot commented Oct 12, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@scheduled-actions-posthog

Copy link
Copy Markdown
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the stale label – otherwise this will be closed in another week. If you want to permanently keep it open, use the waiting label.

@scheduled-actions-posthog

Copy link
Copy Markdown
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant